feat(ssh): make SSH connection robust and self-diagnosing#192
Conversation
Reduces auth dropdown to two conventional options (SSH Config + Password) and rebuilds the connect path so failures surface a concrete cause within seconds instead of opaque 25s timeouts. Key changes: - SshHostResolver: defer host resolution to system `ssh -G` so we inherit Host blocks, Match, Include, IdentityAgent — exactly what `ssh <host>` from a terminal does. - authHandler-driven auth chain: ONE TCP/SSH session, every candidate (multiple agents, IdentityFile entries, default keys) tried in turn the way OpenSSH does. No reconnect-per-key budget burn. - TCP probe before ssh2: catches per-app VPN routing in 5s with an actionable error instead of a 25s "host unreachable". - SFTP open timeout: 8s cap with a server-config diagnostic, since SFTP hangs are a server problem (missing `Subsystem sftp`, restricted shell) that no client retry can fix. - HostName→alias fallback: typing the resolved IP picks up the matching Host block automatically. - Multi-agent enumeration: 1Password, launchctl, env, ~/.ssh/agent.sock all fed to authHandler so launchctl returning the empty system agent doesn't dead-end the chain. - Per-step timing diagnostics in every error: resolve / buildCandidates / tcp probe / tcp+handshake / open sftp, including in-flight steps — the error tells you which step burned the budget. - Encrypted-key detection skips passphrase-protected keys with a clear reason instead of letting ssh2 hang on them. - Robust host-listing parser replaces the ssh-config library which was silently dropping hosts on configs with mixed indentation, comment blocks, or Includes — every Host alias now appears in the dropdown. - Dropdown filter no longer hides other hosts when the input matches an alias exactly (pre-fill case). Persisted profiles with legacy auth values (`auto`, `agent`, `privateKey`) are normalized to `sshConfig` on read so existing configs round-trip cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors SSH authentication to delegate host resolution and credential discovery to the system's OpenSSH client, consolidating several authentication methods into a single sshConfig option. It introduces a more resilient line-based parser for SSH configuration files and adds a TCP reachability probe to improve connection diagnostics. Review feedback identifies a bug in the key-value parser regarding whitespace around equals signs, an issue with relative path resolution in Include directives, and a missing bounds check when parsing cipher lengths in OpenSSH private keys.
| @@ -156,7 +161,6 @@ export class SshConfigParser { | |||
| const expandedPattern = pattern.replace(/^~/, os.homedir()); | |||
There was a problem hiding this comment.
SSH Include directives with relative paths should be resolved relative to ~/.ssh/ (or /etc/ssh/ for root), but the current implementation resolves them relative to the process's current working directory. This can cause includes to fail depending on how the application was launched.
let expandedPattern = pattern.replace(/^~/, os.homedir());
if (!path.isAbsolute(expandedPattern)) {
expandedPattern = path.join(path.dirname(this.configPath), expandedPattern);
}| function parseKeyValue(line: string): { key: string; value: string } | null { | ||
| // Both `Key Value ...` and `Key=Value ...` are valid in OpenSSH config. | ||
| // Avoid regex to keep this linear-time on long lines. | ||
| const eqIdx = line.indexOf('='); | ||
| const wsIdx = findWhitespace(line); | ||
| if (eqIdx !== -1 && (wsIdx === -1 || eqIdx < wsIdx)) { | ||
| const key = line.slice(0, eqIdx).trim(); | ||
| const value = line.slice(eqIdx + 1).trim(); | ||
| if (!key) return null; | ||
| return { key, value }; | ||
| } | ||
| if (wsIdx === -1) return null; | ||
| const key = line.slice(0, wsIdx); | ||
| const value = line.slice(wsIdx + 1).trim(); | ||
| if (!key || !value) return null; | ||
| return { key, value }; | ||
| } |
There was a problem hiding this comment.
The parseKeyValue logic incorrectly handles cases where the key and value are separated by an equals sign with a preceding space (e.g., Host = myhost). In such cases, the = is mistakenly included in the value, which will break host resolution and dropdown population. OpenSSH allows whitespace around the = separator.
function parseKeyValue(line: string): { key: string; value: string } | null {
// Both `Key Value ...` and `Key=Value ...` are valid in OpenSSH config.
// We need to find the first separator which can be '=' or whitespace.
const eqIdx = line.indexOf('=');
const wsIdx = findWhitespace(line);
let sepIdx = -1;
if (eqIdx !== -1 && wsIdx !== -1) {
sepIdx = Math.min(eqIdx, wsIdx);
} else if (eqIdx !== -1) {
sepIdx = eqIdx;
} else if (wsIdx !== -1) {
sepIdx = wsIdx;
} else {
return null;
}
const key = line.slice(0, sepIdx).trim();
let value = line.slice(sepIdx + 1).trim();
// If we split on whitespace, check if the value starts with '=' (e.g., "Key = Value")
if (sepIdx === wsIdx && value.startsWith('=')) {
value = value.slice(1).trim();
}
if (!key || !value) return null;
return { key, value };
}| const cipherLen = buf.readUInt32BE(magic.length); | ||
| const cipher = buf.subarray(magic.length + 4, magic.length + 4 + cipherLen).toString(); |
There was a problem hiding this comment.
When parsing the OpenSSH private key format, it's important to verify that the buffer contains enough data for the declared cipher name length. This prevents potential issues with truncated or malformed key files during the heuristic check.
| const cipherLen = buf.readUInt32BE(magic.length); | |
| const cipher = buf.subarray(magic.length + 4, magic.length + 4 + cipherLen).toString(); | |
| const cipherLen = buf.readUInt32BE(magic.length); | |
| if (buf.length < magic.length + 4 + cipherLen) return false; | |
| const cipher = buf.subarray(magic.length + 4, magic.length + 4 + cipherLen).toString(); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42d25f8450
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| connectConfig.agent = resolved.agent; | ||
| return { | ||
| host: resolved?.hostname ?? config.host, | ||
| port: resolved?.port ?? config.port, |
There was a problem hiding this comment.
When the user/profile supplies a non-default port for a plain host that has no Port in ssh_config, ssh -G still emits the default port 22 (verified with ssh -G example.com), so this line now prefers 22 over config.port. That regresses the existing port field and saved profiles for hosts reachable only on e.g. 2222; keep the previous precedence of an explicit non-22 UI/profile port over the resolver's default.
Useful? React with 👍 / 👎.
| if (value.toLowerCase() !== 'none') { | ||
| result.identityAgent = expandTilde(value); |
There was a problem hiding this comment.
When a Host block sets IdentityAgent none, ssh -G emits identityagent none; the OpenSSH ssh_config docs state that setting the socket name to none disables use of an authentication agent (Debian manpage). Dropping that sentinel means buildAuthCandidates later falls back to SSH_AUTH_SOCK/1Password agents anyway, so hosts that intentionally disable agents can fail before their IdentityFile is tried, especially with low MaxAuthTries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR overhauls SSH connectivity to better match OpenSSH behavior and to surface actionable failure causes quickly, while simplifying the UI/auth model to just SSH Config and Password.
Changes:
- Simplifies persisted/UI auth methods to
sshConfig | passwordand introduces a legacy→current normalization helper. - Rebuilds the main-process SSH connect flow with OpenSSH-based host resolution (
ssh -G), multi-candidate auth, TCP probing, capped SFTP open timeouts, and richer diagnostics. - Replaces SSH host dropdown enumeration with a more robust line-based parser that tolerates real-world
ssh_configformatting/includes.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared/types/notifications.ts | Updates persisted config type unions to the new auth method set. |
| src/shared/types/index.ts | Re-exports normalizeSshAuthMethod for shared consumption. |
| src/shared/types/api.ts | Narrows SshAuthMethod union and adds normalizeSshAuthMethod; documents legacy behavior. |
| src/renderer/store/slices/connectionSlice.ts | Stops persisting privateKeyPath in last-connection state. |
| src/renderer/components/settings/sections/WorkspaceSection.tsx | Updates profile editor UI to the new auth options and normalizes legacy values when editing. |
| src/renderer/components/settings/sections/ConnectionSection.tsx | Updates connect/test UI for new auth options, improves host filtering and error display, normalizes legacy values on prefill/selection. |
| src/main/services/infrastructure/SshHostResolver.ts | New helper to resolve host settings via ssh -G and parse key directives. |
| src/main/services/infrastructure/SshConnectionManager.ts | Major connect-path redesign: TCP probe, auth-candidate chain via authHandler, SFTP timeout, and diagnostic enrichment. |
| src/main/services/infrastructure/SshConfigParser.ts | Reworks getHosts() to use a tolerant line scanner while keeping resolveHost() using ssh-config. |
| src/main/services/infrastructure/index.ts | Exports the new SshHostResolver. |
| src/main/services/infrastructure/ConfigManager.ts | Updates SSH persisted config type for new auth methods. |
| src/main/ipc/ssh.ts | Stops persisting privateKeyPath for last-connection saves via IPC. |
| src/main/ipc/configValidation.ts | Allows legacy auth method values for validation (with intent to normalize). |
| src/main/http/ssh.ts | Stops persisting privateKeyPath for last-connection saves via HTTP. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * 3. Try each candidate in its own ssh2 connection. Auth failure on one | ||
| * candidate moves to the next (matching how OpenSSH behaves). Network | ||
| * errors abort the chain immediately. | ||
| * | ||
| * Two safety nets: | ||
| * - `tryKeyboard: false` on every attempt so the server can't trap us in | ||
| * keyboard-interactive prompts that the GUI can't answer. | ||
| * - A single 20s outer timeout wraps the entire chain (including SFTP open), |
| attempts: AuthAttempt[] | ||
| ): Promise<AuthCandidate[]> { | ||
| if (config.authMethod === 'password') { | ||
| return [{ kind: 'password', password: config.password ?? '', label: 'password' }]; |
| 'Verify with `sftp ' + | ||
| (this.connectedHost ?? '<host>') + | ||
| '` from your terminal — if that also hangs, fix the server config.' |
| // configs round-trip; legacy values are normalized to 'sshConfig' on read. | ||
| const validMethods = ['password', 'sshConfig', 'auto', 'agent', 'privateKey']; | ||
| if (!validMethods.includes(profile.authMethod as string)) return false; |
| username: formUsername.trim(), | ||
| authMethod: formAuthMethod, | ||
| privateKeyPath: formAuthMethod === 'privateKey' ? formPrivateKeyPath.trim() : undefined, | ||
| privateKeyPath: undefined, |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughSSH authentication is refactored to delegate identity resolution to the system SSH client. The authMethod field is narrowed to 'sshConfig' and 'password', privateKeyPath is deprecated, and a new SshHostResolver uses ChangesSSH Config-Based Authentication Migration
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reduces auth dropdown to two conventional options (SSH Config + Password) and rebuilds the connect path so failures surface a concrete cause within seconds instead of opaque 25s timeouts.
Key changes:
ssh -Gso we inherit Host blocks, Match, Include, IdentityAgent — exactly whatssh <host>from a terminal does.Subsystem sftp, restricted shell) that no client retry can fix.Persisted profiles with legacy auth values (
auto,agent,privateKey) are normalized tosshConfigon read so existing configs round-trip cleanly.Summary by CodeRabbit
New Features
Improvements